-
Notifications
You must be signed in to change notification settings - Fork 913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make integration test use standard dist #2867
base: master
Are you sure you want to change the base?
Make integration test use standard dist #2867
Conversation
1bc8a74
to
10296b7
Compare
…om distribution tar ball
10296b7
to
b4b2d0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I left a couple of comments
@@ -46,8 +46,8 @@ RUN mkdir /opt/bookkeeper/conf | |||
RUN mkdir /opt/bookkeeper/scripts | |||
|
|||
### -----Copy Jars------### | |||
ADD ./dist/server.tar.gz /opt/ | |||
RUN mv /opt/server/lib/*.jar /opt/bookkeeper/lib/ | |||
ADD ./dist/bookkeeper-server-*.*SNAPSHOT.tar.gz /opt/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this way the integration tests won't run in a release version branch, correct? (without -SNAPSHOT suffix)
I think we must not break this
I suggest to use
bookkeeper-server-*.tar.gz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, as part of normal CI. It builds release version at location. bookkeeper-dist/server/build/distributions
is built every time, and name of the tarball always contains SNAPSHOT. If you think a situation where name would not contain SNAPSHOT. Sure we can remove. In fact it hardly matters either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should we able to run the tests on the source release, because people would want to build and run tests from the source tarball.
This is very important for a ASF project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I think we are not building src tar ball.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good.
Apart from one comment.
Great work
@@ -46,8 +46,8 @@ RUN mkdir /opt/bookkeeper/conf | |||
RUN mkdir /opt/bookkeeper/scripts | |||
|
|||
### -----Copy Jars------### | |||
ADD ./dist/server.tar.gz /opt/ | |||
RUN mv /opt/server/lib/*.jar /opt/bookkeeper/lib/ | |||
ADD ./dist/bookkeeper-server-*.*SNAPSHOT.tar.gz /opt/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should we able to run the tests on the source release, because people would want to build and run tests from the source tarball.
This is very important for a ASF project
@pkumar-singh any updates? do you still want to working on this? |
Motivation
Current integration tests do not run against docker image being built from distribution tarball. Rather it was building its own tarball and then builds docker image from the tar ball. The tar ball current being generated for running integration test is "similar" but not same compared to distribution tar ball. What integration tests should do is, it should actually run against the exact distribution tar.
Current integration tests was not self contained. That means bookkeeper has to be built before running tests. This PR takes care of that.
Changes
Master Issue: #